Use pushdown params to disable time range constraint pushdown #1216

tlento · 2024-05-15T22:22:50Z

The predicate pushdown operations for time range constriants currently
rely on None/not-None state to determine whether or not to push the
ConstrainTimeRange filter node to the source. With the switch to
pushdown params this None/not-None state gets tenuous, as future
updates will need to do things like manage time range constraints,
other time dimension filters, and categorical dimension (and entity)
filters, and relying on externalizing None/not-None combinations for
these various filter types will be challenging.

This change encapsulates the enabling and disabling of time range
constraints inside of PredicatePushdownParams. At the moment, in
order to maintain the existing behavior, we simply internalize the
None/not-None behavior for time range constraints. This will also
allow us to easily retain pushdown processing for categorical dimensions
even when time constraint filters should not be applied, and
gradually centralize those controls as we streamline the callsites.

There is added complexity to this change because of two things.

Time constraint updating is scattered around in the DataflowPlanBuilder
Conversion metrics currently do predicate pushdown for time constraints

The first of these will be addressed later.
Since we cannot reliably support general predicate pushdown for conversion
metrics, we need to allow for a time-range-only pushdown operation. Today
this is equivalent to pushdown enabled, but that will change shortly.

tlento · 2024-05-15T22:22:58Z

Use pushdown params to disable time range constraint pushdown #1216 : 2 dependent PRs (#1217 , #1218 ) 👈
Encapsulate time range constraints for predicate pushdown #1215
Add conversion metric rendering tests #1214
main

This stack of pull requests is managed by Graphite. Learn more about stacking.

Join @tlento and the rest of your teammates on Graphite

github-actions · 2024-05-15T22:23:08Z

Thank you for your pull request! We could not find a changelog entry for this change. For details on how to document a change, see the contributing guide.

courtneyholcomb

Code looks good! Left some in-line suggestions to improve readability, but feel free to address those at the top of the stack.

courtneyholcomb · 2024-05-16T20:31:48Z

metricflow/dataflow/builder/dataflow_plan_builder.py

@@ -238,21 +242,30 @@ def _build_aggregated_conversion_node(
        constant_properties: Optional[Sequence[ConstantPropertyInput]] = None,
    ) -> BaseOutput:
        """Builds a node that contains aggregated values of conversions and opportunities."""
+        # Pushdown parameters vary with conversion metrics due to the way the time joins are applied.
+        # Due to other outstanding issues with conversion metric fitlers, we disable predicate


nit: fitlers

courtneyholcomb · 2024-05-16T21:03:02Z

metricflow/plan_conversion/node_processor.py

-    time_range_constraint: Optional[TimeRangeConstraint]
+    _PREDICATE_METADATA_KEY = "is_predicate_property"
+
+    time_range_constraint: Optional[TimeRangeConstraint] = dataclasses.field(metadata={_PREDICATE_METADATA_KEY: True})


This last was pretty hard to understand without diving into the dataclasses docs a bit & running the code to see what the value actually is. Might be helpful to leave a comment to explain that this is not a typical default arg (i.e., you still need to pass a value in for time_range_constraint)

Yeah, to be honest I can probably get rid of this and just keep a list of predicate key names. That's what I had originally and I think it's a little less magical overall, but it's still a lot of magic string handling.

courtneyholcomb · 2024-05-16T21:08:59Z

metricflow/plan_conversion/node_processor.py

+        if self.pushdown_state is PredicatePushdownState.FULLY_ENABLED:
+            return
+
+        invalid_predicate_property_names = {


So is this essentially just checking that time_range_constraint is always None when pushdown_state is DISABLED?
If so, this seems pretty complicated & hard to read for just that functionality, and it might be helpful to simplify by just checking that one scenario. But maybe I'm missing something or this gets used more generically down the stack?

Oh yeah, I definitely should've explained that in the commit message. There will be other parameters added which will also need to be nulled out for disabled. Will update things accordingly.

I'm essentially trying to enforce that when the state is disabled there's no possibility of somebody doing something like if pushdown_params.time_range_constraint or pushdown_params.where_filter_specs and then triggering a pushdown operation. If the disabled state was binary I could just make it a derived property but because of our conversion metric and time window handling I have to deal with things like time range only and categorical only pushdown states.

Sounds good!

courtneyholcomb · 2024-05-16T21:14:59Z

metricflow/plan_conversion/node_processor.py

+        )
+
+    @property
+    def is_pushdown_enabled(self) -> bool:


Might be helpful to call this is_pushdown_enabled_for_time_range or something more specific so that other devs don't mistake this as an indication that pushdown is FULLY enabled!

That isn't exactly what this does, but to your point inverting it to is_pushdown_disabled might be better.

I've made a ton of updates and now this is EXACTLY what that does. I also put in the disabled thing but I'll probably remove it. I think I should stop tinkering here and move ahead, though.

tlento

Thanks @courtneyholcomb ! I'm still playing around with the expanded pushdown mechanics so I'll come back to this before merge and either put up another PR or revise this one and request a follow up, depending on what I want to do.

tlento · 2024-05-16T21:27:51Z

metricflow/plan_conversion/node_processor.py

-    time_range_constraint: Optional[TimeRangeConstraint]
+    _PREDICATE_METADATA_KEY = "is_predicate_property"
+
+    time_range_constraint: Optional[TimeRangeConstraint] = dataclasses.field(metadata={_PREDICATE_METADATA_KEY: True})


Yeah, to be honest I can probably get rid of this and just keep a list of predicate key names. That's what I had originally and I think it's a little less magical overall, but it's still a lot of magic string handling.

tlento · 2024-05-16T21:35:18Z

metricflow/plan_conversion/node_processor.py

+        if self.pushdown_state is PredicatePushdownState.FULLY_ENABLED:
+            return
+
+        invalid_predicate_property_names = {


Oh yeah, I definitely should've explained that in the commit message. There will be other parameters added which will also need to be nulled out for disabled. Will update things accordingly.

I'm essentially trying to enforce that when the state is disabled there's no possibility of somebody doing something like if pushdown_params.time_range_constraint or pushdown_params.where_filter_specs and then triggering a pushdown operation. If the disabled state was binary I could just make it a derived property but because of our conversion metric and time window handling I have to deal with things like time range only and categorical only pushdown states.

tlento · 2024-05-16T21:36:53Z

metricflow/plan_conversion/node_processor.py

+        )
+
+    @property
+    def is_pushdown_enabled(self) -> bool:


That isn't exactly what this does, but to your point inverting it to is_pushdown_disabled might be better.

courtneyholcomb

LGTM!

courtneyholcomb · 2024-05-17T01:35:24Z

metricflow/plan_conversion/node_processor.py

@@ -61,7 +61,7 @@ class MultiHopJoinCandidate:
    lineage: MultiHopJoinCandidateLineage


-class PredicatePushdownState(Enum):
+class PushdownPredicateInputType(Enum):


I like this change!!

courtneyholcomb

All looks good!

tlento · 2024-05-24T00:33:21Z

Merge activity

May 23, 5:33 PM PDT: @tlento started a stack merge that includes this pull request via Graphite.
May 23, 5:41 PM PDT: Graphite rebased this pull request as part of a merge.
May 23, 5:46 PM PDT: @tlento merged this pull request with Graphite.

The predicate pushdown operations for time range constriants currently rely on None/not-None state to determine whether or not to push the ConstrainTimeRange filter node to the source. With the switch to pushdown params this None/not-None state gets tenuous, as future updates will need to do things like manage time range constraints, other time dimension filters, and categorical dimension (and entity) filters, and relying on externalizing None/not-None combinations for these various filter types will be challenging. This change encapsulates the enabling and disabling of time range constraints inside of PredicatePushdownParams. At the moment, in order to maintain the existing behavior, we simply internalize the None/not-None behavior for time range constraints. This will also allow us to easily retain pushdown processing for categorical dimensions even when time constraint filters should not be applied, and gradually centralize those controls as we streamline the callsites. There is added complexity to this change because of two things. 1. Time constraint updating is scattered around in the DataflowPlanBuilder 2. Conversion metrics currently do predicate pushdown for time constraints The first of these will be addressed later. Since we cannot reliably support general predicate pushdown for conversion metrics, we need to allow for a time-range-only pushdown operation. Today this is equivalent to pushdown enabled, but that will change shortly.

tlento requested review from courtneyholcomb and plypaul May 15, 2024 22:22

cla-bot bot added the cla:yes label May 15, 2024

This was referenced May 15, 2024

Add conversion metric rendering tests #1214

Merged

Encapsulate time range constraints for predicate pushdown #1215

Merged

Add source_semantic_models property to DataflowPlanNode #1217

Closed

tlento added the Skip Changelog label May 15, 2024

tlento force-pushed the encapsulate-time-range-constraint-for-pushdown branch from 10a9d01 to bfb75ba Compare May 15, 2024 22:47

tlento force-pushed the use-pushdown-params-for-disabling-time-constraint-pushdown branch from 26dd729 to 9435008 Compare May 15, 2024 22:47

courtneyholcomb approved these changes May 16, 2024

View reviewed changes

tlento force-pushed the encapsulate-time-range-constraint-for-pushdown branch from bfb75ba to 55ed28e Compare May 16, 2024 21:45

tlento force-pushed the use-pushdown-params-for-disabling-time-constraint-pushdown branch from 9435008 to 8d77d33 Compare May 16, 2024 21:45

tlento requested a review from courtneyholcomb May 16, 2024 21:45

tlento mentioned this pull request May 16, 2024

Make source_semantic_models property accessible from a DataflowPlanNode #1218

Merged

tlento commented May 16, 2024

View reviewed changes

courtneyholcomb approved these changes May 17, 2024

View reviewed changes

tlento force-pushed the encapsulate-time-range-constraint-for-pushdown branch from 55ed28e to 88723e2 Compare May 22, 2024 01:42

tlento force-pushed the use-pushdown-params-for-disabling-time-constraint-pushdown branch from dc94cfc to 31ee2df Compare May 22, 2024 01:42

tlento requested a review from courtneyholcomb May 22, 2024 01:42

courtneyholcomb approved these changes May 23, 2024

View reviewed changes

tlento force-pushed the encapsulate-time-range-constraint-for-pushdown branch from 88723e2 to ece1e07 Compare May 23, 2024 23:41

tlento force-pushed the use-pushdown-params-for-disabling-time-constraint-pushdown branch from 31ee2df to 3a02ea7 Compare May 23, 2024 23:41

tlento mentioned this pull request May 23, 2024

Add semantic_model_origin property to LinkableElement interface #1230

Merged

tlento force-pushed the encapsulate-time-range-constraint-for-pushdown branch from ece1e07 to 7e723bc Compare May 24, 2024 00:34

Base automatically changed from encapsulate-time-range-constraint-for-pushdown to main May 24, 2024 00:40

tlento added 5 commits May 24, 2024 00:40

Simplify predicate pushdown state management

2668539

Refine time constraint management for predicate pushdown tracking

fb9cc1b

Rename and fixup documentation for predicate input type enumeration

cdf405a

Fitlers is not a word

9769994

tlento force-pushed the use-pushdown-params-for-disabling-time-constraint-pushdown branch from 3a02ea7 to 9769994 Compare May 24, 2024 00:40

tlento merged commit d4bb59c into main May 24, 2024
15 checks passed

tlento deleted the use-pushdown-params-for-disabling-time-constraint-pushdown branch May 24, 2024 00:46

tlento mentioned this pull request Jun 4, 2024

Add integration tests for filters against various join types #1240

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use pushdown params to disable time range constraint pushdown #1216

Use pushdown params to disable time range constraint pushdown #1216

tlento commented May 15, 2024

tlento commented May 15, 2024 •

edited

Loading

github-actions bot commented May 15, 2024

courtneyholcomb left a comment

courtneyholcomb May 16, 2024

courtneyholcomb May 16, 2024

tlento May 16, 2024

courtneyholcomb May 16, 2024

tlento May 16, 2024

courtneyholcomb May 16, 2024

courtneyholcomb May 16, 2024

courtneyholcomb May 16, 2024

tlento May 16, 2024

tlento May 17, 2024

tlento left a comment

tlento May 16, 2024

tlento May 16, 2024

tlento May 16, 2024

courtneyholcomb left a comment

courtneyholcomb May 17, 2024

courtneyholcomb left a comment

tlento commented May 24, 2024 •

edited

Loading

Use pushdown params to disable time range constraint pushdown #1216

Use pushdown params to disable time range constraint pushdown #1216

Conversation

tlento commented May 15, 2024

tlento commented May 15, 2024 • edited Loading

github-actions bot commented May 15, 2024

courtneyholcomb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlento left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

courtneyholcomb left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

courtneyholcomb left a comment

Choose a reason for hiding this comment

tlento commented May 24, 2024 • edited Loading

Merge activity

tlento commented May 15, 2024 •

edited

Loading

tlento commented May 24, 2024 •

edited

Loading